-
Notifications
You must be signed in to change notification settings - Fork 2
W-20893569 adding b2c-cli unit tests #55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
b79fa18 to
c703ca3
Compare
clavery
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments on the style of dependency injection for testing here. I'd like to keep the number of lines supporting tests in the commands to a minimum so if the patterns I mentioned work that would be a better approach I think
| }), | ||
| }; | ||
|
|
||
| protected async deleteCartridges(cartridges: Parameters<typeof deleteCartridges>[1]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this might be to mock the underlying SDK with sinon. But it's adding quite a bit of lines and noise. Can we try simplifying the dependency injection pattern here. Maybe we can support the run method with default arguments set to the imported SDK methods? So the tests can pass in their mocks.
async run(
// we'll have to disable this eslint globally but that's ok
// eslint-disable-next-line unicorn/no-object-as-default-parameter
operations = {
uploadCartridges,
deleteCartridges,
getActiveCodeVersion,
reloadCodeVersion,
},
): Promise<DeployResult> {
const {uploadCartridges, deleteCartridges, getActiveCodeVersion, reloadCodeVersion} = operations;That way only the run method needs to change and the rest of the code stays the same.
Or maybe using a protected member with the operations we want to mock and then mock this in the tests?
protected operations = {
uploadCartridges,
deleteCartridges,
getActiveCodeVersion,
reloadCodeVersion,
};
//
await operations.uploadCartridgesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, Thanks Charles! I’ll look at simplifying the dependency injection pattern for these commands and see how we can reduce the test scaffolding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems very similar to the config-isolation in the SDK. Any way we can share that helper code to keep it consistent?
Summary
Brief description of what this PR does.
This PR delivers a comprehensive unit test suite for the b2c-cli package, covering key CLI commands such as WebDAV, Docs, Job, Sites, MRT, Auth, Code, and ODS.
In addition to expanding test coverage, it standardizes and improves test patterns by introducing shared helpers for:
To enable testing, minor, controlled changes were made to source files of some commands. These test seams allow stubbing internal behavior in isolation without affecting functionality, avoiding reliance on network, filesystem, or local environment.
The PR also consolidates test helper logic by removing ODS-specific helpers and aligning ODS tests with the new common testing approach, improving maintainability and readability across the test suite.
b2c-cli package coverage is now 85%
Testing
How was this tested?
pnpm test)pnpm run format)